Add SQL style linter (vendored Postgres-Extensions/linter) by jnasbyupgrade · Pull Request #16 · Postgres-Extensions/object_reference

jnasbyupgrade · 2026-08-04T23:50:08Z

Vendors Postgres-Extensions/linter as a git submodule at .vendor/linter, with a thin self-initializing lint.mk hand-off so make lint works right after a plain git clone (no --recurse-submodules needed), and wires it into CI as a lint job gated the same way the PG-version test matrix already is (skipped on docs-only pushes).

LINT_TARGETS is scoped to sql/object_reference.sql and test/, excluding the versioned install/update files under sql/ (object_reference--0.1.0.sql, object_reference--stable.sql) — those are frozen/generated per this repo's own convention, so linting them would produce permanent, unfixable findings.
include lint.mk is guarded behind ifneq ($(wildcard .git),): a tarball build (PGXN distribution, or any git archive checkout with no .git) has no submodule to initialize, and Make resolves every include before running any target regardless of which one was requested, so an unguarded rule would break make/make install entirely for a tarball build, not just make lint. Verified with a real git archive HEAD | tar -x checkout: make and make install both succeed with no .git present.
The lint CI job checks out without submodules so the self-init logic in lint.mk is actually exercised, not papered over with a submodules: true checkout.

The first make lint run against sql/object_reference.sql/test/ found 52 real findings, fixed here rather than suppressed:

Most were commented-out SQL written as an ordinary block comment (missing the " * " line prefix, or text after the opening /*) instead of the linter's EXCLUDED CODE disabled-code convention (an alias for sql-lint:disable-block all). Converted each to /* EXCLUDED CODE[: reason], preserving existing TODO: context as the reason where present.
test/helpers/object_table.sql's COPY ... FROM STDIN fixture block is replaced with a plain multi-row INSERT: the linter scans raw COPY payload text for type-name substrings, so its secondary column value integer (pg_catalog's own display name for int4, intentionally preserved as-is in the test data) tripped prefer-short-type. A quoted string literal in an INSERT isn't scanned the same way, so no suppression comment is needed at all.
One genuine prose comment (test/sql/event_trigger.sql) was missing a space after *.

The dead commented-out CREATE TEMP TABLE ... AS alternative in sql/object_reference.sql's ddl_capture exception handler is dropped rather than reformatted — it wasn't a real alternative worth preserving in a comment.

coderabbitai · 2026-08-04T23:50:17Z

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 577ccee0-83cc-4adc-a847-78b2c1b3d439

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

Rebuilt fresh on top of current master, containing only the actual CI/pgxn-tools migration (the SQL style linter integration and the cat_tools 0.3.0 dependency fix that had scope-crept into this branch are split out to #16 and #28 respectively). - Add .github/workflows/ci.yml: a `changes` job (docs-only gate + PG-major-matrix derivation from two constants), a `test` matrix job (container: pgxn/pgxn-tools, PostgreSQL 12-18), and an `all-checks-passed` aggregation gate for use as a single stable required status check. - Remove .travis.yml and pg-travis-test.sh, superseded by the above. - test/dump/run.sh: add -X to several psql invocations, disabling ~/.psqlrc so test runs are deterministic.

Reconciles the substantive feature delta from new_features (PR #2) onto the current 'stable' baseline (post PR #5/#16: pgxn-tools testing, cat_tools 0.3.0, and the linter): - _object_reference._object_oid: drop the per-catalog regclass/regconfig/ regdictionary/regnamespace/regoperator/regprocedure/regtype columns and their unique indexes plus the count_nulls-backed null_count trigger that enforced "exactly one is set". classid is now plain oid and object_oid (also NOT NULL) is the sole identifier column, so there's nothing left to arbitrate between. - _object_reference._object_v / _object_v__for_update: drop the reg* columns from the column list to match. - _object_reference._object_oid__add: replace the dynamic, format()-built INSERT that picked a reg* column based on cat_tools.object__reg_type() with a plain INSERT into object_oid. - Drop the count_nulls search_path DO block (dead now that the trigger using it is gone) and the count_nulls dependency throughout (control, Makefile, test setup). - Add object_reference.object__describe()/object__identity(), thin wrappers around pg_describe_object()/pg_identify_object(); and object__cleanup(), which best-effort deletes an object record (ignoring foreign_key_violation if it's still referenced elsewhere). Wire object__cleanup() up to a new AFTER DELETE trigger on object_group__object so removing an object from its last group automatically attempts cleanup. - _object_v__for_update (the getsert core): refuse to track objects living in a pg_temp*/pg_toast_temp* schema, since a tracked reference would outlive the temporary object it points to. - test/sql/object_group.sql: switch the two scratch tables from TEMP to regular tables (object__getsert now rejects temp objects) and add coverage for the new automatic-cleanup trigger. - test/sql/base.sql: replace the count_nulls-relocation test (relocation was already unsupported and the whole extension no longer depends on count_nulls) with coverage for object_oid, object__describe(), object__identity(), and temp-object rejection. sql/object_reference--0.1.0.sql (the frozen historical release) and the META files are untouched. default_version stays 'stable'; sql/object_reference--stable.sql is regenerated to match sql/object_reference.sql. make lint and make test (including the dump/restore test) pass on both PostgreSQL 12 and 17. Supersedes PR #2 (new_features) and, for the update/upgrade test infrastructure built on top of it, sets up the rebuild of PR #3. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

0.1.0 and stable are byte-identical for this function's body once the linter's annotation (PR #16's own, separate content) isn't part of this branch -- the CREATE OR REPLACE was a no-op here. Verified via bin/test_existing's structural diff (clean) and a full local run (7/7 pgTAP files, dump/restore, test-build).

Reconciles the substantive feature delta from new_features (PR #2) onto the current 'stable' baseline (post PR #5/#16: pgxn-tools testing, cat_tools 0.3.0, and the linter): - _object_reference._object_oid: drop the per-catalog regclass/regconfig/ regdictionary/regnamespace/regoperator/regprocedure/regtype columns and their unique indexes plus the count_nulls-backed null_count trigger that enforced "exactly one is set". classid is now plain oid and object_oid (also NOT NULL) is the sole identifier column, so there's nothing left to arbitrate between. - _object_reference._object_v / _object_v__for_update: drop the reg* columns from the column list to match. - _object_reference._object_oid__add: replace the dynamic, format()-built INSERT that picked a reg* column based on cat_tools.object__reg_type() with a plain INSERT into object_oid. - Drop the count_nulls search_path DO block (dead now that the trigger using it is gone) and the count_nulls dependency throughout (control, Makefile, test setup). - Add object_reference.object__describe()/object__identity(), thin wrappers around pg_describe_object()/pg_identify_object(); and object__cleanup(), which best-effort deletes an object record (ignoring foreign_key_violation if it's still referenced elsewhere). Wire object__cleanup() up to a new AFTER DELETE trigger on object_group__object so removing an object from its last group automatically attempts cleanup. - _object_v__for_update (the getsert core): refuse to track objects living in a pg_temp*/pg_toast_temp* schema, since a tracked reference would outlive the temporary object it points to. - test/sql/object_group.sql: switch the two scratch tables from TEMP to regular tables (object__getsert now rejects temp objects) and add coverage for the new automatic-cleanup trigger. - test/sql/base.sql: replace the count_nulls-relocation test (relocation was already unsupported and the whole extension no longer depends on count_nulls) with coverage for object_oid, object__describe(), object__identity(), and temp-object rejection. sql/object_reference--0.1.0.sql (the frozen historical release) and the META files are untouched. default_version stays 'stable'; sql/object_reference--stable.sql is regenerated to match sql/object_reference.sql. make lint and make test (including the dump/restore test) pass on both PostgreSQL 12 and 17. Supersedes PR #2 (new_features) and, for the update/upgrade test infrastructure built on top of it, sets up the rebuild of PR #3. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

0.1.0 and stable are byte-identical for this function's body once the linter's annotation (PR #16's own, separate content) isn't part of this branch -- the CREATE OR REPLACE was a no-op here. Verified via bin/test_existing's structural diff (clean) and a full local run (7/7 pgTAP files, dump/restore, test-build).

github-actions · 2026-08-10T22:10:35Z

Claude finished @jnasbyupgrade's task in 2m 33s —— View job

Claude finished the review

Todo list

Gather context (diff, CLAUDE.md, submodule/lint.mk setup)
Review CI workflow changes
Review Makefile / lint.mk / submodule wiring
Review SQL changes (object_reference.sql, test/*)
Post final review

Summary

This is a clean, well-scoped PR. I read through the full diff (git diff origin/master...HEAD), the vendoring mechanism, and confirmed CI (including the new lint job) already passed on this commit (run 31437042187) — so the self-init/no---recurse-submodules path is proven to work in practice, not just in theory.

CI wiring (.github/workflows/ci.yml)

lint job correctly gated behind needs.changes.outputs.docs_only != 'true', same as test, and added to all-checks-passed's needs list — consistent with the existing pattern.
Deliberately checking out without submodules for the lint job is the right call — it's what actually exercises lint.mk's self-init rather than assuming it works.

Makefile / lint.mk

The ifneq ($(wildcard .git),) guard is correct and necessary — Make resolves include before dispatching to any target, so an unguarded include lint.mk would break make install for tarball/PGXN builds. I confirmed sql/ only currently contains object_reference.sql and object_reference--0.1.0.sql, so LINT_TARGETS = sql/object_reference.sql test/ correctly excludes the frozen versioned file as intended.
The .vendor/linter/lint.mk: ... rule + include .vendor/linter/lint.mk self-init idiom is a standard, correct Make pattern (Make will build a missing included file via its rule before re-reading it).
Minor/non-blocking nit: the Makefile comment says the frozen-install-file convention is documented "in this repo's CLAUDE.md / memory" — the current CLAUDE.md doesn't actually mention that convention (it only points to the cross-repo ../ai/CLAUDE.md). Not a functional issue, just a slightly inaccurate comment reference if CLAUDE.md was meant literally.
One design note (not a defect): pgxntool is vendored in this repo as plain committed files, while the new linter is vendored as an actual git submodule. That's a reasonable choice given the linter is a separate actively-maintained project, but it does mean the repo now has two different vendoring conventions side by side — worth a one-line mention in a README/CONTRIBUTING if one exists, purely for future-contributor clarity.

SQL changes

All the commented-out-SQL → /* EXCLUDED CODE[: reason] conversions (sql/object_reference.sql, test/deps.sql, test/sql/capture.sql, test/sql/object_group.sql) look faithful to the original intent, and existing TODO: context was preserved as the reason.
test/helpers/object_table.sql: verified the COPY ... FROM STDIN → INSERT ... VALUES conversion preserves semantics exactly, including that empty-string fields (not \N) map to '' rather than NULL in every row — this matters because a later UPDATE ... SET secondary = nullif(secondary, '') in the same file depends on secondary being an empty string, not NULL, for rows where it wasn't set. That invariant holds correctly in the new INSERT.
The dropped dead CREATE TEMP TABLE ... AS alternative in the ddl_capture exception handler was already-unreachable commented-out code with no distinguishing value — fine to delete outright rather than reformat.
test/sql/event_trigger.sql's comment-spacing fix is trivial and correct.

No bugs, correctness issues, or security concerns found. Nice attention to detail in the PR description explaining why each mechanical choice (the .git guard, the no-submodule CI checkout, the INSERT-vs-COPY distinction for prefer-short-type) was made.

Vendor Postgres-Extensions/linter as a git submodule at .vendor/linter, following the same pattern already adopted in cat_tools: a thin self-initializing lint.mk hand-off (so `make lint` works right after a plain `git clone`, no --recurse-submodules needed), LINT_TARGETS scoped to sql/object_reference.sql and test/ (excluding the frozen, never hand-edited versioned install files under sql/, e.g. object_reference--0.1.0.sql/--stable.sql), and a CI job that runs `make lint` directly -- the same entry point a developer uses locally -- so the self-init logic is actually exercised, not just the rule checking. The `include lint.mk` is guarded on .git being present: a tarball build (PGXN distribution, `git archive` with no .git) has no submodule to initialize, and Make resolves every `include` before running any target regardless of which one was requested, so an unguarded rule would break `make`/`make install` entirely for a tarball build, not just `make lint`. Fixes the real pre-existing style findings this first run turned up (52 total): most were commented-out SQL marked as prose comments instead of using the linter's `EXCLUDED CODE` disabled-code convention (missing " * " prefixes flagged as comment-line-prefix/comment-opening violations); one COPY data block's `secondary` column intentionally mirrors pg_catalog's own type display name ("integer" for int4) rather than following prefer-short-type, so it's suppressed via a scoped disable-block region instead of being "fixed" into incorrect test data. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

- sql/object_reference.sql: the commented-out CREATE TEMP TABLE ... AS alternative in the ddl_capture exception handler was dead weight, not a real alternative worth preserving in a comment. - test/helpers/object_table.sql: the disable-block was working around the linter scanning raw COPY payload text for type-name substrings. A quoted string literal isn't scanned at all, so a plain multi-row INSERT needs no suppression -- same data, properly quoted.

github-actions · 2026-08-10T22:17:11Z

Claude finished @jnasbyupgrade's task in 1m 37s —— View job

Claude finished the review

Todo list

Gather context (diff, CLAUDE.md, submodule/lint.mk setup)
Review CI workflow changes
Review Makefile / lint.mk / submodule wiring
Review SQL changes (object_reference.sql, test/*)
Post final review

Summary

Reviewed the full diff (git diff origin/master...HEAD, 11 files) against origin/master. CI on this exact commit is green (31437520466), including the new lint job, confirming the self-init/no---recurse-submodules path works in practice.

CI wiring (.github/workflows/ci.yml)

lint job is gated behind needs.changes.outputs.docs_only != 'true' and included in all-checks-passed's needs, consistent with the existing test job pattern.
Checking out the lint job without submodules is the right call — it's the only way to actually exercise lint.mk's self-init logic instead of assuming it.

Makefile / lint.mk

ifneq ($(wildcard .git),) guard is correct and necessary: Make resolves every include before dispatching to any target, so an unguarded include lint.mk would break make install for a tarball/PGXN build, not just make lint.
LINT_TARGETS = sql/object_reference.sql test/ correctly excludes the frozen versioned install file (object_reference--0.1.0.sql) per this repo's existing convention (documented via pgxntool's README.asc reference in the comment — accurate as written).
.vendor/linter/lint.mk: ... + include .vendor/linter/lint.mk is a standard, correct "build the include via its own rule" Make idiom.
Design note (non-blocking): pgxntool is vendored as plain committed files while the new linter is vendored as a real git submodule — two vendoring conventions side by side. Reasonable given the linter is a separately-maintained project, but worth a one-line note somewhere for future contributors if this pattern isn't already documented.

SQL changes

All commented-out-SQL → /* EXCLUDED CODE[: reason] conversions (sql/object_reference.sql, test/deps.sql, test/sql/capture.sql, test/sql/object_group.sql) faithfully preserve original intent, including existing TODO: context carried into the reason text.
test/helpers/object_table.sql: the COPY ... FROM STDIN → INSERT ... VALUES conversion preserves semantics exactly — verified every row's empty-string fields map to '' (not NULL), which matters because the later SET secondary = nullif(secondary, '') (line 183) depends on secondary being '', not NULL, for unset rows. That invariant holds. The added inline comment explaining why 'integer' (not 'int') is intentional for prefer-short-type is a nice touch.
The dropped dead CREATE TEMP TABLE ... AS alternative in the ddl_capture exception handler was unreachable, undocumented commented-out code — fine to delete rather than reformat into EXCLUDED CODE.
test/sql/event_trigger.sql's comment-spacing fix (*Rename → * Rename) is trivial and correct.

No bugs, correctness issues, or security concerns found. This PR is well-scoped, and the description clearly explains the reasoning behind each mechanical choice (the .git guard, the no-submodule CI checkout, the INSERT-vs-COPY distinction). LGTM.

…instead Reverts the earlier INSERT-VALUES rewrite. Wraps the original COPY ... FROM STDIN payload in a sql-lint:disable-block prefer-short-type / enable-block region instead -- the raw text triggers 2 real findings ("integer"/"int4" inside data, not type references) that the rewrite worked around by restructuring the data instead of just telling the linter to ignore this specific, known-safe case. Co-Authored-By: Claude <noreply@anthropic.com>

github-actions · 2026-08-10T22:38:06Z

Claude finished @jnasbyupgrade's task in 1m 37s —— View job

Claude finished reviewing

Todo list

Gather context (diff, CLAUDE.md, submodule/lint.mk setup)
Review CI workflow changes
Review Makefile / lint.mk / submodule wiring
Review SQL changes (object_reference.sql, test/*)
Post final review

Summary

This push (90dbcf9) reverts the earlier COPY→INSERT rewrite in test/helpers/object_table.sql and replaces it with a sql-lint:disable-block prefer-short-type / sql-lint:enable-block pair around the original COPY ... FROM STDIN payload instead. I diffed this against origin/master and against the state reviewed previously.

test/helpers/object_table.sql (the only substantive change since the last review)

Verified via git show origin/master:test/helpers/object_table.sql that the COPY block content (lines 148–162) is now byte-for-byte identical to the original pre-PR code — this is a pure "tell the linter to ignore it" fix, not a data rewrite. That's arguably cleaner than the prior INSERT-based workaround, since it avoids restructuring test data just to dodge a false positive, and directly documents why the finding is safe to ignore (integer/int4 appear in COPY payload data, not as real type references).
Confirmed CI's 🧹 SQL Lint job passed on this exact commit (run 31438969738), which confirms the linter does honor sql-lint:disable-block/enable-block directives placed in -- line comments, not only inside /* */ block comments — worth knowing since every other suppression in this repo uses the /* EXCLUDED CODE */ block-comment alias instead. Both forms now coexist in the codebase; not a problem, just a minor stylistic inconsistency (this is the only spot using the raw directive instead of the EXCLUDED CODE alias, presumably because the EXCLUDED CODE alias disables the whole block including the real COPY statement, whereas here only the literal payload lines need suppressing while the COPY/\. structure remains live SQL).
All other files (.github/workflows/ci.yml, .gitmodules, Makefile, lint.mk, sql/object_reference.sql, test/deps.sql, test/sql/capture.sql, test/sql/event_trigger.sql, test/sql/object_group.sql) are unchanged since the prior two reviews on this PR, both of which found no issues:
- lint CI job correctly gated the same way as test (docs_only != 'true') and included in all-checks-passed's needs.
- Checking out the lint job without submodules is the right call — it's what actually proves lint.mk's self-init path works.
- The .git-presence guard around include lint.mk in the Makefile is correct and necessary: Make resolves include before dispatching any target, so an unguarded include would break make install for tarball/PGXN builds.
- EXCLUDED CODE conversions elsewhere are faithful to original intent, including preserved TODO: context.

No bugs, correctness issues, or security concerns found. LGTM.

This was referenced Aug 5, 2026

Switch to pgxn-tools based testing #5

Merged

Remove reg* pseudotype columns; add object info functions #17

Open

jnasbyupgrade mentioned this pull request Jul 30, 2026

Add update+upgrade (U&U) test foundation: test/install, load-mode switch, TEST_SCHEMA, dependency guard jnasbyupgrade/object_reference#3

Closed

jnasbyupgrade closed this Aug 10, 2026

jnasbyupgrade reopened this Aug 10, 2026

jnasbyupgrade force-pushed the add-linter branch 2 times, most recently from 60eb192 to 159f6c9 Compare August 10, 2026 22:08

jnasbyupgrade and others added 2 commits August 10, 2026 17:14

jnasbyupgrade force-pushed the add-linter branch from 159f6c9 to 7e03424 Compare August 10, 2026 22:15

jnasbyupgrade merged commit 94a9272 into Postgres-Extensions:master Aug 10, 2026
12 checks passed

Navigation Menu

Uh oh!

Add SQL style linter (vendored Postgres-Extensions/linter) - #16

Add SQL style linter (vendored Postgres-Extensions/linter)#16
jnasbyupgrade merged 3 commits into
Postgres-Extensions:masterfrom
jnasbyupgrade:add-linter

jnasbyupgrade commented Aug 4, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Aug 4, 2026 •

edited

Loading

Review skipped

Uh oh!

github-actions Bot commented Aug 10, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Aug 10, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Aug 10, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

jnasbyupgrade commented Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

github-actions Bot commented Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Claude finished the review

Summary

Uh oh!

github-actions Bot commented Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Claude finished the review

Summary

Uh oh!

github-actions Bot commented Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Claude finished reviewing

Summary

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

jnasbyupgrade commented Aug 4, 2026 •

edited

Loading

coderabbitai Bot commented Aug 4, 2026 •

edited

Loading

github-actions Bot commented Aug 10, 2026 •

edited

Loading

github-actions Bot commented Aug 10, 2026 •

edited

Loading

github-actions Bot commented Aug 10, 2026 •

edited

Loading